CLDSRV-855: Update helpers#6128
Merged
tmacro merged 4 commits intoimprovement/CLDSRV-869/account_limitingfrom Apr 9, 2026
Merged
CLDSRV-855: Update helpers#6128tmacro merged 4 commits intoimprovement/CLDSRV-869/account_limitingfrom
tmacro merged 4 commits intoimprovement/CLDSRV-869/account_limitingfrom
Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Review by Claude Code |
❌ 105 Tests Failed:
View the top 3 failed test(s) by shortest run time
To view more test analytics, go to the Test Analytics Dashboard |
tcarmet
approved these changes
Apr 1, 2026
lib/Config.js
Outdated
|
|
||
| // Rate limiting uses the same localCache config defined for S3 to avoid config duplication. | ||
| // If rate limiting is enabled check to make sure it is also configured. | ||
| if (config.rateLimiting?.enabled) { |
Contributor
There was a problem hiding this comment.
Any reason why we don't do something like this now?
Suggested change
| if (config.rateLimiting?.enabled) { | |
| if (this.rateLimiting.enabled) { |
|
LGTM |
| code: errors.SlowDown.message, | ||
| message: errors.SlowDown.description, | ||
| }), | ||
| }).default({ |
Contributor
There was a problem hiding this comment.
The serviceUserArn is required but doesn't appear in the global default value, is it wanted?
Comment on lines
-64
to
-66
| const applyRateLimit = config.rateLimiting?.enabled | ||
| && !rateLimitApiActions.includes(request.apiMethod) // Don't limit any rate limit admin actions | ||
| && !request.isInternalServiceRequest; // Don't limit any calls from internal services |
Contributor
There was a problem hiding this comment.
This seems to be done by requestNeedsRateCheck now, but extractAccountRateLimitConfig doesn't call this function, is it expected?
|
|
||
| function buildRateChecksFromConfig(resourceClass, resourceId, limitConfig) { | ||
| const checks = []; | ||
| if (limitConfig?.RequestsPerSecond?.Limit !== undefined && limitConfig.RequestsPerSecond?.Limit > 0) { |
Contributor
There was a problem hiding this comment.
Suggested change
| if (limitConfig?.RequestsPerSecond?.Limit !== undefined && limitConfig.RequestsPerSecond?.Limit > 0) { | |
| if (limitConfig?.RequestsPerSecond?.Limit > 0) { |
BourgoisMickael
approved these changes
Apr 2, 2026
| @@ -1860,23 +1860,15 @@ class Config extends EventEmitter { | |||
| this.enableVeeamRoute = config.enableVeeamRoute; | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
parseRateLimitConfig is now called unconditionally, but serviceUserArn is Joi.string().required() in the schema. If a user has { rateLimiting: { enabled: false } } without serviceUserArn, this will now throw. Previously it worked because parseRateLimitConfig was only called when enabled: true.
Fix: in config.js line 178, make serviceUserArn conditionally required:serviceUserArn: Joi.string().when('enabled', { is: true, then: Joi.required() })
— Claude Code
| log.debug('Rate limit cleanup completed', { expiredConfigs, expiredBucketOwners }); | ||
| } | ||
|
|
||
| cleanupTimer = setTimeout(() => cleanupJob(log), rateLimitCleanupInterval); |
There was a problem hiding this comment.
cleanupJob recursively calls itself without passing options, so skipUnref is lost after the first run. In production this is harmless (options is always empty), but in tests with skipUnref: true the second and subsequent timeouts will call unref() unexpectedly.
Pass options through: setTimeout(() => cleanupJob(log, options), rateLimitCleanupInterval)
— Claude Code
Review by Claude Code |
tmacro
force-pushed
the
improvement/CLDSRV-855/update_helpers
branch
from
b0b9212 to
73691f0
Compare
Review by Claude Code |
| if (!options.skipUnref) { | ||
| cleanupTimer.unref(); | ||
| } | ||
| } |
There was a problem hiding this comment.
Switching from setInterval to recursive setTimeout without a try/catch means if expireCachedConfigs() or expireCachedBucketOwners() ever throws, the cleanup loop silently dies and never restarts — leading to a slow memory leak from accumulated expired entries. setInterval was resilient to callback exceptions.
Wrap the body in try/catch to keep the loop alive on unexpected errors.
— Claude Code
| }, | ||
| }; | ||
| // Parse and validate all rate limiting configuration | ||
| this.rateLimiting = parseRateLimitConfig(config.rateLimiting); |
There was a problem hiding this comment.
Backward compatibility issue: parseRateLimitConfig is now always called. If a deployment has "rateLimiting": { "enabled": false } in config.json without serviceUserArn, startup will crash because the Joi schema has serviceUserArn: Joi.string().required(). Previously, parseRateLimitConfig was only called when enabled was true.
Consider making serviceUserArn conditionally required (only when enabled: true) in the Joi schema.
— Claude Code
Review by Claude Code |
tmacro
force-pushed
the
improvement/CLDSRV-855/update_helpers
branch
from
405a417 to
185e209
Compare
|
LGTM |
tmacro
merged commit 2be2b25
into
improvement/CLDSRV-869/account_limiting
18 of 27 checks passed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.